-
Notifications
You must be signed in to change notification settings - Fork 498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Arnau's MongoDB API #485
base: master
Are you sure you want to change the base?
Arnau's MongoDB API #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Arnau, your code is well-written and covers the essential aspects of building a RESTful API, I see no issues in the code but I couldn't test all the endpoints, I gave an internal error back after trying several times. I can't see from the code how was the DB populated, did you test it and did you manage to get the right responses back?Regardless, your implementation almost meets the basic requirements: fix the /books/:id endpoint and you passed! 🥂
mongoose.connect(mongoUrl); | ||
//This will connect us to the Data Base | ||
const mongoUrl = process.env.MONGO_URL || "mongodb://localhost/books"; | ||
mongoose.connect(mongoUrl, { useNewUrlParser: true, useUnifiedTopology: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! did you know that with new Mongoose version you could just write mongoose.connect(mongoUrl)
? 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No i didn't, thank's! I feel like I have to improve the documentation part, I'm still green on looking for it.
mongoose.Promise = Promise; | ||
|
||
//Here we create a Book modell | ||
const Book = mongoose.model("Book", { | ||
bookID: Number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove bookId
from the Schema: Mongoose automatically assigns a unique _id
field to each document, so you don't need to define the field in your schema, you can even remove it from the data object
app.get("/books/:id", async (req, res) => { | ||
const { id } = req.params; | ||
try { | ||
const book = await Book.findOne({ bookID: id }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're trying to find the _id
value accessing it with bookID
and that's why it's not working. It should look like:
const book = await Book.findOne({ _id: id });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The datajson cames with a "bookID" and I got confused, nice you noticed, now it's working:)
Thank's for the code review Antonella! The other end point that wasn't working is books/rating, I just copied and pasted this hard code without understanding it. |
Render link
here
Solo
[github-vidalhuix]